-
Notifications
You must be signed in to change notification settings - Fork 290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change tool metadata file format to JSON #1553
Conversation
…ting trivial destruction.
…the search loop, and get rid of magic numbers that is the length of the table.
Make the table be static data, deduplicate part of the expression in the search loop, and get rid of magic numbers that is the length of the table.
5fe30dc
to
3747fae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feature!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add a json schema for this file?
"name": "cmake", | ||
"os": "linux", | ||
"version": "3.22.2", | ||
"executable": "cmake-3.22.2-linux-x86_64/bin/cmake", | ||
"url": "https://github.com/Kitware/CMake/releases/download/v3.22.2/cmake-3.22.2-linux-x86_64.tar.gz", | ||
"sha512": "579e08b086f6903ef063697fca1dc2692f68a7341dd35998990b772b4221cdb5b1deecfa73bad9d46817ef09e58882b2adff9d64f959c01002c11448a878746b", | ||
"archive": "cmake-3.22.2linux-x86_64.tar.gz" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this entry has no architecture entry this version would be used on arm64 devices. Maybe the arch field should be mandatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's reasonable for there to be arch-less entries like scripts blobs or similar. A big reason to do this work is to enable arm64, but truly doing well there is going to imply minting arm64 binaries and similar; fully getting that on sounds like a separate feature though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok i hadn't thought about scripts, so one must simply be careful when adding entries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(or user "arch" : "neutral" as an indicator for stuff which does not dependent on arch similar to how the vs channel manifest works)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think an explicit 'neutral' value is substantially clearer than just not having the field, and would be 'annoying' as we couldn't reuse the existing CPUArchitecture
enum and friends.
EDIT: Oh I see it's necessary due to
I don't believe this is needed because all this stuff is already tested by the fetch tests. |
Continuation of #1490
Related: #1277
This PR contains several changes:
vcpkgTools.xml
data into a JSON file and removes the XML parsing code.